Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support splitting a single item into multiple files #270

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

missingdays
Copy link

Motivation

To allow generating code for a single entity in multiple files. See a related issue in volo - cloudwego/volo#454

Solution

Introduce a new split mode that can be enabled using the builder

@missingdays missingdays requested review from a team as code owners September 23, 2024 15:48
@CLAassistant
Copy link

CLAassistant commented Sep 23, 2024

CLA assistant check
All committers have signed the CLA.

@missingdays
Copy link
Author

missingdays commented Sep 23, 2024

I've only added a single test so far, considering this is a proposal and not a final solution.
When we agree on how the final solution should look like, I will implement the remaining tests.

@missingdays missingdays force-pushed the split-multiple-files branch 3 times, most recently from e2e114b to 4c26560 Compare September 24, 2024 07:53
Copy link
Member

@PureWhiteWu PureWhiteWu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much for this contribution!

I think we should add the type prefix to those generated files, since different types may not be in the same resolve namespace. For example, we can have a service A and struct A at the same time.

pilota-build/src/lib.rs Outdated Show resolved Hide resolved
@Millione
Copy link
Member

LGTM. Maybe a test for the workspace mode can be added?

@missingdays
Copy link
Author

missingdays commented Sep 24, 2024

@Millione Hi. I tried adding a test for a workspace, but couldn't find any existing tests for it. And my test fails with a panic at pilota-build/src/codegen/workspace.rs:91 because Cargo.toml doesn't exist. I'm not sure how this file should be created when generating the workspace for the first time. Here's how I compile with the workspace mode

      crate::Builder::thrift()
          .ignore_unused(false)
          .compile_with_config(
              vec![IdlService::from_path(source.to_owned())],
              crate::Output::Workspace(target.into()),
          )

Could you please point me to an existing test for a workspace mode, or help me figure out why Cargo.toml is expected to already exist in group_defs?

@Millione
Copy link
Member

Could you please point me to an existing test for a workspace mode

Unfortunately, there is no existing test for this, but I'll do my best to help you figure it out.

Why Cargo.toml is expected to already exist in group_defs?

The difference between normal mode and workspace mode is that in workspace mode the code generation doesn't take place in the build phase, but in the run phase. It generates multiple crates and adds all these crates as workspace members to the Cargo.toml file.

I'm not sure how this file should be created when generating the workspace for the first time

In my opinion, we should put the workspace mode test in a separate tests folder, so that it is a package with a binary target and fits into the real usage.

@missingdays
Copy link
Author

I added tests for the workspace mode, as well as splitting the files in the workspace mode.
I added all the tests from the thrift tests, I'm not sure if so many are needed or if it's too much

@Millione
Copy link
Member

I added tests for the workspace mode, as well as splitting the files in the workspace mode.

Fantastic jobs!

I added all the tests from the thrift tests, I'm not sure if so many are needed or if it's too much

Maybe that's a bit too much? I think one thrift test is enough, but the tested thrift files should be well-constructed for the workspace, e.g. A.thrift and B.thrift both use C.thrift struct, and see if the struct is generated in common crates and used correctly by A and B.

@PureWhiteWu
Copy link
Member

A.thrift and B.thrift both use C.thrift struct

Maybe more complex? for example, more layers of nesting mods/namespaces?

@missingdays
Copy link
Author

Could you maybe write down the .thrift file you have in mind? My knowledge of thrift is quite limited

@PureWhiteWu
Copy link
Member

Sorry for the late reply, I've constructed five idl files.

article.thrift:

include "image.thrift"
include "author.thrift"
include "common.thrift"

namespace rs article

enum Status {
    NORMAL = 0,
    DELETED = 1,
}

struct Article {
    1: required i64 id,
    2: required string title,
    3: required string content,
    4: required author.Author author,
    5: required Status status,
    6: required list<image.Image> images,
    7: required common.CommonData common_data,
}

struct GetArticleRequest {
    1: required i64 id,
}

struct GetArticleResponse {
    1: required Article article,
}

service ArticleService {
    GetArticleResponse GetArticle(1: GetArticleRequest req),
}

author.thrift:

include "image.thrift"
include "common.thrift"

namespace rs author

struct Author {
    1: required i64 id,
    2: required string username,
    3: required string email,
    4: required image.Image avatar,
    5: required common.CommonData common_data,
}

struct GetAuthorRequest {
    1: required i64 id,
}

struct GetAuthorResponse {
    1: required Author author,
}

service AuthorService {
    GetAuthorResponse GetAuthor(1: GetAuthorRequest req),
}

cdn.thrift:

include "common.thrift"

namespace rs article.image.cdn

struct CDN {
    1: required i64 id,
    2: required string url,
    3: required common.CommonData common_data,
}

common.thrift:

namespace rs common

struct CommonData {
    1: required i64 id,
    2: required string name,
    3: required string description,
}

image.thrift:

include "common.thrift"
include "cdn.thrift"

namespace rs article.image

struct Image {
    1: required i64 id,
    2: required string url,
    3: required cdn.CDN cdn,
    4: required common.CommonData common_data,
}

struct GetImageRequest {
    1: required i64 id,
}

struct GetImageResponse {
    1: required Image image,
}

service ImageService {
    GetImageResponse GetImage(1: GetImageRequest req),
}

And there are three services that can be used in workspace mode, in article.thrift, author.thrift, image.thrift.

@missingdays
Copy link
Author

@PureWhiteWu Thanks, I've replaced the workspace tests with the one you provided, it seems to work fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants